Skip to content

[Ma Jiajun] iP#197

Open
Jamarcus111 wants to merge 24 commits intonus-cs2113-AY2324S2:masterfrom
Jamarcus111:master
Open

[Ma Jiajun] iP#197
Jamarcus111 wants to merge 24 commits intonus-cs2113-AY2324S2:masterfrom
Jamarcus111:master

Conversation

@Jamarcus111
Copy link

No description provided.

Copy link

@aditichadha1310 aditichadha1310 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, good job!

@@ -0,0 +1,40 @@
public class TaskFactory {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid long methods. Take corrective action when the method exceeds 30 lines of code.

throw new HandleException("The input cannot be empty!");
} else if (userInput.equalsIgnoreCase("list")) {
taskList.listTasks();
} else if (!userInput.startsWith("todo") && !userInput.startsWith("deadline") && !userInput.startsWith("event")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fairly complex statement. You might want to break this into sub-expressions and store values into some boolean variables to check for the "if" condition.

}

public void addTask(String userInput) throws HandleException {
// Similar to addTask method in TaskManager, but refactored for this class

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure the comments are for the reader and not private notes for yourself. It should help in increasing code readability.

String type = parts[0];
Task task = null;

if (parts.length < 2 || parts[1].isEmpty()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the use of Magic numbers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants